-
Notifications
You must be signed in to change notification settings - Fork 422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebSockets for netty-cats #3628
Conversation
server/netty-server/cats/src/main/scala/sttp/tapir/server/netty/cats/NettyCatsServer.scala
Show resolved
Hide resolved
.../netty-server/src/main/scala/sttp/tapir/server/netty/internal/ReactiveWebSocketHandler.scala
Outdated
Show resolved
Hide resolved
.../netty-server/src/main/scala/sttp/tapir/server/netty/internal/ReactiveWebSocketHandler.scala
Outdated
Show resolved
Hide resolved
.../netty-server/src/main/scala/sttp/tapir/server/netty/internal/ReactiveWebSocketHandler.scala
Outdated
Show resolved
Hide resolved
.../netty-server/src/main/scala/sttp/tapir/server/netty/internal/ReactiveWebSocketHandler.scala
Outdated
Show resolved
Hide resolved
server/netty-server/src/main/scala/sttp/tapir/server/netty/NettyConfig.scala
Outdated
Show resolved
Hide resolved
...tty-server/src/main/scala/sttp/tapir/server/netty/internal/ws/ReactiveWebSocketHandler.scala
Outdated
Show resolved
Hide resolved
...tty-server/src/main/scala/sttp/tapir/server/netty/internal/ws/ReactiveWebSocketHandler.scala
Outdated
Show resolved
Hide resolved
server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/package.scala
Outdated
Show resolved
Hide resolved
server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/package.scala
Show resolved
Hide resolved
server/tests/src/main/scala/sttp/tapir/server/tests/ServerWebSocketTests.scala
Show resolved
Hide resolved
server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyServerHandler.scala
Show resolved
Hide resolved
@adamw Adding proper handling for non-ws responses in ws endpoints made me think that it's better to remove |
@kciesielski yeah if that's feasible, sounds good, might clean up the code :) Not sure about extracting the code to a WS package, you would loose the possibility to call the 500/400 response-generators then? But maybe these should be simply put in yet another "common" object, |
Hmm such extractions require further refactoring, because response handlers are internal implicit classes referencing fields of |
@kciesielski let's skip them then ;) |
otherwise this PR will end up being gigantic feature + refactor |
I'm totally on the same page on this 😁 |
I'm reading through the codebase to get a better mental model of this, but since I'm generally not familiar with it (nor with websockets in detail, for that matter), it might take me some time until I can submit some non-superficial review. Not sure if there's any point in waiting for me :) |
I think in the end code is even simpler now. Great work - let's ship it :) |
This PR adds support for Web Socket endpoints in
tapir-netty-cats
, with all the sub-features likeautoPongOnPing
,ignorePong
,decodeCloseRequests
,decodeCloseResponses
, andautoPing
.Some remarks:
Fs2StreamCompatible
, andWebSocketPipeProcessor
. Especially the latter, which creates a Reactive StreamsProcessor[Req, Resp]
from user-providedfs2.Pipe[F, Req, Resp]
.WebSocketPipeProcessor
has a workaround to properly handle exceptions thrown in the pipeline. It needs to prevent underlying cancelation mechanisms and instead signal back an error to the Netty channel handler, so that the server properly sends a Close frame. Otherwise the channel closes silently, which also happens in some other servers, like Vert.X, so tests for error handling run only whenfailingPipe
is set to true inServerWebSocketTests
.In performance tests, latency is on par with our fastest backends (like http4s):